-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LG-13136: Fix rate limiting logic when user is on last try #10538
Conversation
@@ -28,7 +28,7 @@ def status | |||
:unauthorized | |||
elsif document_capture_session.cancelled_at | |||
:gone | |||
elsif rate_limiter.limited? | |||
elsif rate_limiter.exceed_max? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking whether we should completely remove this check, due to the way rate_limiter
implemented.
And technically this controller
wont be calle, since when doc_auth
is submitted then the rate_limit
should not be exceeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying out removing this call locally. Everything seems to be passing so far.
My main concern is that - if we shouldn't ever call this controller, is there some sort of earlier check that's misfiring? Because if it shouldn't have ever been called, then this user should never have encountered this issue...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@night-jellyfish , i meant this wont be called technically if rate limited, unless something goes wrong or called intentionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point me to the earlier check that would filter out rate limited requests? I am having a hard time finding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@night-jellyfish here, it's a controller
before_action
hook
before_action :confirm_not_rate_limited, except: [:update] |
also
before_action :confirm_not_rate_limited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!! I also found this relevant conversation that further supports your point.
With that in mind, I updated this PR to instead delete the check for rate limiting, as you suggested. I still need to test this manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I need to also add a feature test for this, perhaps.
When we used `limited?` we found the user was prevented from completing their last chance at a request, due to the `>=`, since the attempts and max_attempts were equal. Instead we needed a way to only prevent that if the user had exceeded their max amount of tries.
In the previous commit, we were looking to prevent the `capture_doc_status_controller` from setting the rate limit too early. In this commit, we are instead removing the logic to even look at rate limiting here, since [as Dawei pointed out](#10538 (comment)), and as I found [a past discussion for](#9370 (comment)), this code should never be reached if rate limited.
7753cb3
to
67569af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I get rate limited during they hybrid flow, the desktop is stuck on the polling page. It'd help to add a feature test for hybrid rate limiting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to successfully complete all the steps in the testing plan. I can approve this once you address Amir's feedback.
@amirbey When you were testing the hybrid flow and got rate limited, did the desktop still display the |
hi @eileen-nava ... yes, that screen remains unchanged after the user gets rate limited |
@amirbey @night-jellyfish I followed up with Kelli and realized that we had a Figma and ticket for a different issue. Kelli documented the past behavior in this figma and Team Ada fixed it in LG-9813. |
I pushed up a commit to restore the original test that looked for that page. Unfortunately I'm struggling to get both tests to pass. @amirbey or @dawei-nava perhaps we could look at it tomorrow? |
timer = JobHelpers::Timer.new | ||
# user submit a request, set the requested_at timestamp | ||
document_capture_session.requested_at = Time.zone.now | ||
document_capture_session.save! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
persist it immediately in case we get racing condition in poller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This timestamp is used at another place
document_capture_session.requested_at = Time.zone.now |
In this case, guess there is no racing concern, so no need to persist immediately.
Also, should we reset it after doc auth since it use used at this place also, maybe not.
# doc auth failed due to non network error or doc_pii is not valid | ||
if client_response && !client_response.success? && !client_response.network_error? | ||
errors_hash = client_response.errors&.to_h || {} | ||
failed_front_fingerprint = nil | ||
failed_back_fingerprint = nil | ||
if errors_hash[:front] || errors_hash[:back] | ||
if errors_hash[:front] | ||
selfie_image_fingerprint = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need explicitly save failed result and result timestamp when finger print check disabled.
@@ -21,10 +22,16 @@ def self.mock_response!(method:, response:) | |||
@response_mocks[method.to_sym] = response | |||
end | |||
|
|||
def self.response_delay(delay) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding delays in mock client to facilitate tests that reflect reality, useful for hybrid flow poller testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea. 👍🏻
@@ -142,20 +140,25 @@ | |||
|
|||
before do | |||
allow(IdentityConfig.store).to receive(:doc_auth_max_attempts).and_return(max_attempts) | |||
allow(IdentityConfig.store).to receive(:doc_auth_check_failed_image_resubmission_enabled). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disable check so we can resubmit same images, easier to test.
# final failure | ||
# reset to return mocked normal success response for the last attempt | ||
DocAuth::Mock::DocAuthMockClient.reset! | ||
DocAuth::Mock::DocAuthMockClient.response_delay(8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add delays longer than a polling cycle which is 5 sec.
547f055
to
7ff8083
Compare
7ff8083
to
9a22735
Compare
Future works:
|
@@ -98,10 +98,15 @@ def had_barcode_attention_result? | |||
end | |||
|
|||
def redo_document_capture_pending? | |||
return true if !session_result && document_capture_session&.requested_at.present? | |||
return unless session_result&.dig(:captured_at) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may take a detailed look, is it necessary? what's our assumption when this method is invoked? what are the edge cases?
Team Timnit paused work on this bug in anticipation of the upcoming team transition. The comments provide guidance for whoever picks it up in the future. Thanks, @night-jellyfish and @dawei-nava, for all your work on this bug! |
🎫 Ticket
LG-13136
🛠 Summary of changes
When we used
limited?
we found the user was prevented from completing their last chance at a request, even if the request came back from TrueID as a success. Due to the use of>=
inlimited?
, in this situation the attempts and max_attempts were equal. While polling for the results, the user would be short-circuited to thetoo_many_requests
logic.Instead we needed a way to only prevent that if the user had exceeded their max amount of tries.
📜 Testing Plan
doc_auth_max_attempts
to a low number locally in yourapplication.yml